Fix year calculation for the last day of a leap year. #1550
Conversation
|
/azp run |
|
No pipelines are associated with this pull request. |
| if (year1 == 4) | ||
| { | ||
| // this is the last day in a leap year | ||
| year1 = 3; | ||
| } |
There was a problem hiding this comment.
Given this is being ported here, I want to understand it a bit better:
Azure/azure-sdk-for-cpp#1254
However, it has an edge case where the current year is the last day of a leap year, such that
SecondsIn4Yearscontains that extra leap day but the calculation ofyeardidn't account for the current date being exactly that extra day.
I don't fully follow what you mean here. Can you please elaborate, with a concrete example. Also, what do you mean by the "current year is the last day of a leap year"?
Lastly, what is an example impact of this bug exactly to an end user?
cc @antkmsft
There was a problem hiding this comment.
I don't fully follow what you mean here. Can you please elaborate, with a concrete example.
2020-12-31 results in year1 being 4, despite this code expecting all blocks of 4 years to be handled by SecondsIn4Years.
Also, what do you mean by the "current year is the last day of a leap year"?
Should be current "current day is the last day of a leap year"
Lastly, what is an example impact of this bug exactly to an end user?
There was a problem hiding this comment.
Lastly, what is an example impact of this bug exactly to an end user?
It incorrectly prints Thursday 2020-12-31 as Thu 2021-01-01
| @@ -736,6 +736,12 @@ static compute_year_result compute_year(int64_t secondsSince1900) | |||
| int secondsInt = static_cast<int>(secondsLeft - year4 * SecondsIn4Years); | |||
|
|
|||
| int year1 = secondsInt / SecondsInYear; | |||
| if (year1 == 4) | |||
| { | |||
| // this is the last day in a leap year | |||
There was a problem hiding this comment.
This comment is a little confusing since we are talking about a day, but the variable is year.
|
Hi, |
Not processing leap seconds is intentional. The input is a unix timestamp, and unix time does not respect leap seconds. If it did account for leap seconds that would break the consistency of timestamps; because leap seconds are not issued at a fixed interval, it would change the meaning of timestamps before or after they were added to any such accounting table or similar.
I'm not seeing an obvious bug here related to the year 2100. The "leap years are those divisible by 4 but not divisible by 10" rule is why this is handled in 400 year cycles in the first place (and why the NT epoch is 1601). |
calculate_yeardetermines the year by following the gregorian calendar's rules of breaking years into 400, 100, and 4 year cycles. However, it has an edge case where the current year is the last day of a leap year, such thatSecondsIn4Yearscontains that extra leap day but the calculation ofyeardidn't account for the current date being exactly that extra day.Since we expect anything in a 4 year cycle to be handled by
SecondsIn4Years, ifyearends up being 4, we know it is that last day of a leap year, and as such get the correct result by using 3 instead. The subsequent code that uses a month day table according to a year's leap-ness then handles the result correctly.